Make auction creative rewriting optional#916
Conversation
Allow operators to retain sanitizer-accepted external URLs in POST /auction adm while preserving mandatory server-side sanitization and the existing default behavior.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
The runtime branch itself is well scoped: sanitization remains mandatory, rewriting is gated after sanitization, both modes have substantive tests, and /first-party/proxy remains independent. I am requesting changes for rollback compatibility and two operator-facing configuration/privacy contracts described in the inline comments.
Non-blocking
🏕 camp site
- Update the internal auction README:
crates/trusted-server-core/src/auction/README.md:139,:251, and:382still describe creative rewriting as unconditional. Please consider updating those passages to distinguish mandatory sanitization from default-on, configurable rewriting.
📌 out of scope
- Track the pre-existing
iframe[srcdoc]sanitizer gap: the generic handler atcrates/trusted-server-core/src/creative.rs:385neither removes nor recursively sanitizessrcdoc, while the normal renderer grantsallow-scriptsandallow-same-originatcrates/trusted-server-js/lib/src/core/render.ts:14. This exists at the base SHA and should not block this PR, but it deserves a security follow-up that removessrcdocand adds regression coverage through both rewrite modes.
👍 praise
- The implementation keeps sanitization strictly before the configuration branch, avoids logging creative contents, covers default/disabled behavior and legacy blob loading, and verifies that proxy HTML/CSS rewriting is independent. No new dependency, OS API, or WASM-incompatible construct is introduced.
CI Status
- fmt and all adapter/target clippy checks: PASS
- Rust tests and builds (Fastly, Axum, Cloudflare, Spin, parity, CLI): PASS
- JS formatting and Vitest: PASS
- integration, browser, Fastly EC lifecycle, and CodeQL checks: PASS
|
Implemented and pushed the requested review fixes in Also addressed the non-blocking internal auction README cleanup. The pre-existing nested Validation completed locally:
|
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Adds a default-true [auction].rewrite_creatives setting: creative sanitization for POST /auction stays mandatory, while first-party resource/click rewriting and TSJS injection become optional. Backward-compatibility handling (default omitted from serialized/legacy blobs, explicit false preserved, legacy schema round-trip, EdgeZero env-overlay pre-existing-leaf quirk) is thorough and well tested. No blocking issues found.
Non-blocking
⛏ nitpick
- "unre-written" wording:
docs/guide/configuration.mdanddocs/guide/creative-processing.mdrepeat the hyphenation "unre-written" (e.g. "sanitized but unre-written HTML") several times. Suggest "not rewritten" or "unrewritten" for readability — no behavior impact.
👍 praise
- Backward-compatibility test coverage: the
skip_serializing_ifdefault-omission design plus tests acrosssettings.rs(TOML omitted/explicit-false),config_payload.rs(legacy JSON blob round-trip and legacy-schema deserialization), and the new CLI integration test (config_env_overlay.rs, proving the EdgeZero "env overlay only overrides pre-existing leaves" quirk is handled) directly cover the real rollback/migration edge cases operators will hit. Theproxy.rstest proving/first-party/proxyrewriting stays independent of the new flag is a good isolation check too.
📝 note
- Change table is stale: the PR description's file table lists 12 files; the actual diff touches 16, including
creative.rs(see inline comment — an independent exclusion-matching bug fix bundled here),crates/trusted-server-cli/tests/config_env_overlay.rs,crates/trusted-server-core/src/auction/README.md, anddocs/guide/cli.md. Worth syncing the table with the real diff before merge.
CI Status
- fmt: PASS
- clippy (fastly/axum/cloudflare native+wasm/spin native+wasm): PASS
- rust tests (fastly/axum/cloudflare/spin/cross-adapter parity/CLI): PASS
- js tests (vitest): PASS
- js/docs format: PASS
| || lower.starts_with("about:") | ||
| { | ||
| // Match exclusions against the same absolute URL used for rewriting. | ||
| if settings.rewrite.is_excluded(&absolute) { |
There was a problem hiding this comment.
📌 out of scope — This changes to_abs() to build the absolute URL before calling settings.rewrite.is_excluded(), instead of checking exclusion on the raw token. That's a real behavior fix: is_excluded() calls url::Url::parse(), which fails on a schemeless string like //trusted-cdn.example.com/x.js, so previously an exclude_domains entry never actually excluded a creative URL that used the protocol-relative form — it got rewritten anyway despite being configured as excluded. It's well tested (new cases around line 1317+), but it's an independent bug fix bundled into the "make rewriting optional" PR and isn't mentioned in the Change table. Worth calling out explicitly in the PR description since it changes existing exclusion behavior on main for protocol-relative URLs.
| let sanitized = creative::sanitize_creative_html(raw_creative); | ||
| let rewritten = creative::rewrite_creative_html(settings, &sanitized); | ||
| let sanitized_len = sanitized.len(); | ||
| let rewrite_creatives = settings.auction.rewrite_creatives; |
There was a problem hiding this comment.
🤔 thinking — settings.auction.rewrite_creatives is read fresh on every iteration of the per-bid loop; it's loop-invariant for the whole response. Trivial cost, not blocking — just flagging that hoisting it above the for loop would be marginally cleaner.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Narrowly-scoped, backward-compatible feature with strong test coverage (default, explicit false, TOML parse, blob round-trip, legacy-schema acceptance, CLI env overlay, and a regression test proving /first-party/proxy rewriting is unaffected) and careful documentation. Two items need attention before merge: an undeclared drive-by behavior change in to_abs, and the fact that the rollback-critical skip_serializing_if carries no in-code rationale. Both are inline.
Blocking
🔧 wrench
- Undeclared behavior change in
to_abs: moving the exclusion check onto the normalized URL is a real fix —is_excludednever matched protocol-relative URLs becauseurl::Url::parsefails on//host/path— but it is unrelated torewrite_creativesand appears in neither the PR description nor the CHANGELOG. It also silently tightens the/first-party/proxytsurlvalidator (proxy.rs:1633), which is untested. (crates/trusted-server-core/src/creative.rs:53-73) skip_serializing_ifis load-bearing but unexplained:AuctionConfigisdeny_unknown_fields, so omitting the default is what keeps pushed blobs readable by an older binary during rollback. That rationale exists only in docs and a TOML comment, so a future cleanup of the attribute silently breaks rollback. (crates/trusted-server-core/src/auction_config_types.rs:15-19)
❓ question
- Client-side behavior with
rewrite_creatives = false: disabling also dropsdata-tsclickand thetsjs-unified.min.jsruntime injection. The docs frame this purely as a privacy trade-off. Is the creative render bridge tolerant of a missing runtime, or does the creative fail to size/render? If rendering depends on it, that is a functional consequence and belongs in the warning block indocs/guide/creative-processing.md. ts config diffwith the leaf present: raised inline ontrusted-server.example.toml:119.
Non-blocking
♻️ refactor
- Gate the flag inside
creative:crates/trusted-server-core/src/auction/formats.rs:256-268(inline). - Warn when the privacy default is off:
crates/trusted-server-core/src/auction/formats.rs:258(inline).
🤔 thinking
- Hand-mirrored
LegacyAuctionConfigwill drift:crates/trusted-server-core/src/config_payload.rs:52-72(inline). - CLI test does not pin the working directory:
crates/trusted-server-cli/tests/config_env_overlay.rs:36-63(inline).
📌 out of scope
/first-party/proxysign handler bypassesto_absfor protocol-relative URLs: the//-prefixed branch builds the absolute URL inline and never callsto_abs, so it skips the exclusion check entirely — the exact inconsistency this PR fixes on the creative path. Pre-existing; worth a follow-up issue now that the sibling path was corrected. (crates/trusted-server-core/src/proxy.rs:1624-1632)[debug].inject_adm_for_testingembeds raw, unsanitizedadm: correctly documented here as debug-only, but it remains the one creative path with neither sanitization nor rewriting. Follow-up issue. (crates/trusted-server-core/src/publisher.rs:2144-2147)
⛏ nitpick
rewrite_modestring is unnecessary:crates/trusted-server-core/src/auction/formats.rs:264-268(inline).
Verification notes
- The docs change from "env vars baked at build time" to "typed CLI overlay" was verified accurate: no
TRUSTED_SERVER__handling remains incrates/trusted-server-core/build.rs. [debug].inject_adm_for_testingreferenced in the new docs exists as described (settings.rs:1923).convert_to_openrtb_responsehas exactly two production callers, both inauction/endpoints.rs(219, 340), so the new flag's blast radius matches what the docs claim.
CI Status
Taken from the PR's own checks — not re-run locally.
- fmt: PASS
- clippy / CodeQL (
Analyze (rust),Analyze (javascript-typescript),Analyze (actions)): PASS - rust tests (
cargo test, axum native, cloudflare, spin, ts CLI native, cross-adapter parity): PASS - js tests (vitest), format-typescript, format-docs: PASS
- integration tests, Fastly EC lifecycle, browser integration tests: still pending at review time
| } else { | ||
| None | ||
| } | ||
| Some(absolute) |
There was a problem hiding this comment.
🔧 wrench — Undeclared behavior change: this reorder is a real fix, but it is unrelated to rewrite_creatives and is not listed in the PR description or the CHANGELOG.
RewriteConfig::is_excluded (settings.rs:553) parses with url::Url::parse, which fails on //host/path and returns false — so protocol-relative URLs were never excluded before. Matching against the normalized absolute URL corrects that, and changes two user-visible behaviors:
- Creatives —
//excluded-cdn.example/x.pngis now left direct instead of proxied. /first-party/proxy—to_absis also thetsurlvalidator atproxy.rs:1633, so URLs on excluded domains that previously signed successfully now fail withunsupported url.
Fix: add a CHANGELOG ### Fixed entry and a PR-description line covering both effects, or split this into its own PR. The new tests at creative.rs:1317+ cover the creative path; nothing covers the proxy-validator side effect.
| default = "default_rewrite_creatives", | ||
| skip_serializing_if = "is_default_rewrite_creatives" | ||
| )] | ||
| pub rewrite_creatives: bool, |
There was a problem hiding this comment.
🔧 wrench — skip_serializing_if here is load-bearing for binary rollback, but nothing in the code says so.
AuctionConfig is #[serde(deny_unknown_fields)] (line 8), so a pushed blob carrying rewrite_creatives hard-fails config load on a binary that predates the field. skip_serializing_if is the only thing preventing that — and the rationale currently lives solely in a docs page and a TOML comment. The next person who "simplifies" this attribute silently breaks rollback for every deployment.
Fix:
/// Rewrite sanitized winning-bid creative HTML to first-party endpoints.
///
/// `skip_serializing_if` is deliberate, not cosmetic: [`AuctionConfig`] is
/// `deny_unknown_fields`, so a pushed blob carrying this key fails config load
/// on a binary predating it. Omitting the default keeps blobs
/// rollback-compatible; an explicit `false` must serialize and therefore pins
/// the deployment to a binary that knows the field.
#[serde(
default = "default_rewrite_creatives",
skip_serializing_if = "is_default_rewrite_creatives"
)]
pub rewrite_creatives: bool,Worth repeating a one-line pointer on is_default_rewrite_creatives (line 69) too, since that is where the deletion would start.
| # environment override. Set false to return sanitized but unre-written winning-bid | ||
| # adm, skipping proxy/click URL conversion and creative TSJS injection. | ||
| # Sanitization is always applied. Restore and push true before an older-binary rollback. | ||
| rewrite_creatives = true |
There was a problem hiding this comment.
❓ question — Does ts config diff stay clean once this leaf is present?
The example now ships rewrite_creatives = true explicitly (needed for the EdgeZero v0.0.4 overlay, which does not create missing leaves), but config push serializes it away via skip_serializing_if. If config diff compares the typed loader's output on both sides this is a no-op; if it compares the TOML document against the stored blob, every operator sees a phantom diff on every run.
Could you confirm which it is? If it is the latter, an assertion in crates/trusted-server-cli/tests/config_env_overlay.rs covering config diff would pin it.
| let processed = if rewrite_creatives { | ||
| creative::rewrite_creative_html(settings, &sanitized) | ||
| } else { | ||
| sanitized |
There was a problem hiding this comment.
♻️ refactor — "Always sanitize, conditionally rewrite" is a policy, but it lives as an inline if inside one converter. A second auction-creative emitter would silently miss it, and the invariant cannot be unit-tested where the functions actually live.
Fix: move it into creative so it has a single home:
// creative.rs
/// Sanitize auction creative markup, then rewrite it to first-party endpoints
/// unless `[auction].rewrite_creatives` is disabled. Sanitization is mandatory
/// in both modes.
pub fn process_auction_creative(settings: &Settings, raw: &str) -> String {
let sanitized = sanitize_creative_html(raw);
if settings.auction.rewrite_creatives {
rewrite_creative_html(settings, &sanitized)
} else {
sanitized
}
}That also collapses the sanitized_len and rewrite_mode temporaries at the call site.
| let sanitized = creative::sanitize_creative_html(raw_creative); | ||
| let rewritten = creative::rewrite_creative_html(settings, &sanitized); | ||
| let sanitized_len = sanitized.len(); | ||
| let rewrite_creatives = settings.auction.rewrite_creatives; |
There was a problem hiding this comment.
♻️ refactor — Disabling this flag turns off the product's core first-party mediation promise, but the only signal is a debug! line per bid — invisible at production log levels.
Fix: emit a one-shot log::warn! at config load when auction.enabled && !auction.rewrite_creatives, e.g. "Auction creative rewriting disabled; creative assets and clicks will contact third-party hosts directly". That makes an accidental false visible without trawling debug logs.
| "enabled" | ||
| } else { | ||
| "disabled" | ||
| }; |
There was a problem hiding this comment.
⛏ nitpick — rewrite_mode as "enabled"/"disabled" can just be the bool: rewrite={} with rewrite_creatives reads the same and drops the four-line let.
| _creative_store: String, | ||
| #[serde(rename = "allowed_context_keys")] | ||
| _allowed_context_keys: std::collections::HashSet<String>, | ||
| } |
There was a problem hiding this comment.
🤔 thinking — This hand-mirrored copy of the pre-rewrite_creatives schema is the right idea, but nothing keeps it in sync. Every future AuctionConfig field needs a matching field added here, or the test quietly starts passing for the wrong reason (it would still accept the payload — it just would no longer be asserting what its name claims).
A comment naming the release this snapshot represents (e.g. // Mirrors AuctionConfig as of v0.x — do not add fields introduced after that.) would at least make the drift obvious in review.
| .arg(&project.config_path) | ||
| .env(REWRITE_ENV, raw_value) | ||
| .output() | ||
| .expect("should run ts config validate") |
There was a problem hiding this comment.
🤔 thinking — Neither this helper nor the config push invocation pins a working directory, so Command inherits the crate's cwd while the assertion reads .edgezero/local-config-trusted_server_config.json from the tempdir. It passes on CI, so the store evidently resolves relative to --manifest — but that is an edgezero-cli implementation detail, and if it ever changes the test either fails confusingly or writes .edgezero/ into the repo.
Fix: add .current_dir(project.directory.path()) to both commands.
3 participants
Summary
[auction].rewrite_creativessetting for winning-bidadmreturned byPOST /auction./first-party/proxyHTML/CSS rewriting and document the behavior and privacy trade-offs.Changes
crates/trusted-server-core/src/auction_config_types.rsrewrite_creativeswith explicit serde and Rust defaults oftrue.crates/trusted-server-core/src/auction/orchestrator.rscrates/trusted-server-core/src/auction/formats.rscrates/trusted-server-core/src/auction/endpoints.rs/auctionresponses.crates/trusted-server-core/src/settings.rscrates/trusted-server-core/src/config_payload.rscrates/trusted-server-core/src/proxy.rstrusted-server.example.tomldocs/guide/configuration.mddocs/guide/auction-orchestration.mddocs/guide/creative-processing.mdCHANGELOG.mdCloses
Closes #914
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo doc --package trusted-server-core --no-deps --all-features(completed with existing unrelated rustdoc warnings)Checklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)